Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MT-Bench and PR-Bench Support #9

Merged
merged 6 commits into from
Jun 25, 2024
Merged

Conversation

danmcp
Copy link
Member

@danmcp danmcp commented Jun 17, 2024

No description provided.

@danmcp danmcp force-pushed the main branch 23 times, most recently from 7b11486 to a9c16f2 Compare June 19, 2024 20:24
@danmcp danmcp changed the title WIP: Add MT-Bench Support Add MT-Bench Support Jun 19, 2024
Copy link
Member

@nathan-weinberg nathan-weinberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial round of comments, will look more in-depth as well

requirements.txt Show resolved Hide resolved
src/instructlab/eval/answers.py Outdated Show resolved Hide resolved
src/instructlab/eval/common.py Outdated Show resolved Hide resolved
src/instructlab/eval/common.py Outdated Show resolved Hide resolved
src/instructlab/eval/common.py Outdated Show resolved Hide resolved
src/instructlab/eval/common.py Outdated Show resolved Hide resolved
Comment on lines 26 to 28
with open(fn, "r", encoding="utf-8") as file:
contents = yaml.safe_load(file)
return contents.get("seed_examples")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we checking the YAML/schema validity at all?

Don't think we need for this PR specifically, but @bjhargrave is working on a Dev Doc to get this functionality into the instructlab-schema package: instructlab/dev-docs#101

@danmcp danmcp force-pushed the main branch 13 times, most recently from 55e6346 to 1594a73 Compare June 24, 2024 23:43
for _ in range(API_MAX_RETRY):
try:
messages = conv.to_openai_api_messages()
if messages[0]["role"] == "system" and messages[1]["role"] == "user":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xukai92 do we need to change this to what's in here? xukai92/FastChat@5d44295. You have an issue for it here: #11

Copy link
Member Author

@danmcp danmcp Jun 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and included. Where are we expecting the env var to be set though?

README.md Outdated Show resolved Hide resolved
tests/test_branch_gen_answers.py Show resolved Hide resolved
@alimaredia
Copy link
Contributor

The comments I had in my review are not meant to block the merging of this PR. They are to point out or ask questions about follow up work.

@danmcp danmcp force-pushed the main branch 7 times, most recently from d9289cf to 3e21919 Compare June 25, 2024 15:42
Copy link
Contributor

@JamesKunstle JamesKunstle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR seems like it's in good enough shape to start iterating on.

@nathan-weinberg nathan-weinberg merged commit ffe1aa1 into instructlab:main Jun 25, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants